NSEC3 and multiple key signing support. #416

ximon18 · 2024-10-15T14:18:02Z

Currently lacks collision detection and tests, though has been manually tested using ldns-verify-zone, dnssec-verify and named-checkzone both with and without opt-out and also including both signed and unsigned delegations.

I'm posting this here as a draft to allow for alignment and early feedback from the team working on various pieces of DNSSEC support for domain.

Note: This PR includes the content of #444.

bal-e · 2024-10-16T07:04:56Z

src/sign/ring.rs

+pub fn nsec3_hash<N, SaltOcts, HashOcts>(
+    owner: N,
+    algorithm: Nsec3HashAlg,
+    iterations: u16,
+    salt: &Nsec3Salt<SaltOcts>,
+) -> Result<OwnerHash<HashOcts>, Nsec3HashError>


Why not just take the owner name and an Nsec3param?

nsec3_default_hash can then be replaced by calling this function with default() for the second argument.

Why not just take the owner name and an Nsec3param?

I had that but took it out. There was a reason. I'll see if I can remember why.

Ah I know why. The nsec3s() function takes an Nsec3Param struct which it uses to create the NSEC3PARAM RR at the apex of the zone. For the NSEC3PARAM RR RFC 5155 says that the opt-out flag in the flags field MUST be zero, so for RFC compliance the Nsec3Param struct passed to nsec3s() should have the opt-out flag set to zero.

Honestly I think this is a bit of a foot-gun and perhaps best not to pass an Nsec3Param to nsec3s() but instead only the other fields (algorithm, iterations, salt), however MAYBE in future it will be legal to set some of the other flag bits in the flags field and a user would want to have those set in the created NSEC3PARAM RR... so for that reason nsec3s() currently takes an Nsec3Param as input.

When generating NSEC3 RRs, and when opt-out is enabled, the flags value in the given Nsec3Param cannot be used as-is because the opt-out flag must be set to 1 (but NOT in the NSEC3PARAM RR), and rather than copy the given Nsec3Param or modify it and then pass it to nsec3_hash() I felt it was better to just pass only the values actually needed for hashing in, as NSEC3 hashing doesn't need the flags field at all, and also that way users don't have to think about what value to set the unused Nsec3Param::flags field to when invoking nsec3_hash() directly (as dnst nsec3-hash does).

bal-e · 2024-10-16T07:08:30Z

src/sign/ring.rs

+    let owner_hash = OwnerHash::from_octets(hash)
+        .map_err(|_| Nsec3HashError::OwnerHashError)?;


I don't think this should return an OwnerHashError. There are two failure cases for from_octets: if the hash is more than 255 bytes (impossible since NSEC3 doesn't support any such digest algorithms) or if memory could not be allocated (in which case we should return AppendError).

It's not clear to me from the docs on OwnerHashError that it only fails relating to length, it just says "The hashing process produced an invalid owner hash" and I have no way of knowing when that error might occur or why.

src/sign/ring.rs

src/sign/records.rs

src/sign/ring.rs

ximon18 · 2024-10-30T12:31:36Z

@bal-e: I realize that I moved the nsec3_hash() function from validator/nsec.rs to sign/ring.rs I think because that more clearly showed that it was Ring powered/dependent. However, we were discussing yesterday that lots of functionality is shared between signing and validating and that right now you are putting everything under validation, while I proposed a common module shared by both signing and validating. Either way, if you are currently putting everything under validation, this move to sign is inconsistent and perhaps you think I should move it back?

bal-e · 2024-10-30T16:49:47Z

@ximon18: yeah, I think this should be moved under validate for now. We'll gather up the shared functionality in that module for now, then decide how to distribute it.

ximon18 · 2024-10-30T23:13:27Z

@ximon18: yeah, I think this should be moved under validate for now. We'll gather up the shared functionality in that module for now, then decide how to distribute it.

Done.

src/sign/records.rs

bal-e · 2024-10-31T10:45:02Z

src/sign/records.rs

+            //    the apex and the original owner name."
+            let distance_to_root = name.owner().iter_labels().count();
+            let distance_to_apex = distance_to_root - apex_label_count;
+            if distance_to_apex > 1 {


I don't think the if statement is necessary, the for loop will run for zero iterations if this condition is not true.

True, but the if statement matches nicely with the RFC text and makes it clear that if we enter this block it is because of the condition identified by the RFC text.

src/sign/records.rs

src/validate.rs

…ve-replace

…-zonemd-remove-replace-plus-pr444

…ve-replace

…-zonemd-remove-replace-plus-pr444

…ve-replace

…-zonemd-remove-replace-plus-pr444

E.g. LDNS seems to consider DS and CDS and CDNSKEY resource record types as well as DNSKEY when selecting keys.

…ve-replace

…rn just the record types they produce.

…SortedRecords.

…cts.

Philip-NLnetLabs · 2025-01-29T15:49:10Z

src/sign/keys/signingkey.rs

+    ///
+    /// The range spans from the inception timestamp up to and including the
+    /// expiration timestamp.
+    signature_validity_period: Option<RangeInclusive<Timestamp>>,


This is not really a property of a key and can get confusing of something is signed by multiple keys. It should be removed and the signature validity should be part of the signing interfaces.

Hmm, yes, I see what you mean. This was fine for the initial use case of one-shot signing, but makes no sense for using the same key later to re-sign expired signatures. And mabe there are other reasons why it makes no sense. I'll change this.

…y, as the validity period required can change over the lifetime of a key and may be affected by other factors such as current signature expiration time and jitter.

src/sign/signatures/rrsigs.rs

…m the NSEC unit test suite with the changes required for the NSEC3 case.

…enerate_nsecs(). - Added a unit test verifying compliance with the RFC 5155 Appendix A signed zone example. - Imroved and added comments. - Rewrote cryptic slice based NSEC3 next owner hashing loop with initial (untested!) type bit map merging and collision detection support, and removed no longer needed (but added in this branch) SortedRecords::as_mut_slice(). - Removed confusing duplicate storage of NSEC3 opt-out info in GenerateNsec3Config. - Added missing (though not breaking) trailing periods in test data.

bal-e

This is a massive PR, @ximon18, well done on getting all this functionality together into a cohesive unit!

My primary concern is with the large number of new traits added. I'm worried that they're trying to provide extensibility in a manner that won't actually be helpful for our target use cases. In some places, they can be replaced with simpler concrete data types, and I'd strongly prefer that.
A lot of public methods (e.g. in sign::records) are still missing documentation. Perhaps a Clippy lint can help catch everything?
There are a number of similarly named types and traits (e.g. DesignatedSigningKey, DnssecSigningKey, and SigningKey). Can some of these be combined together to reduce ambiguity in the public API? I'm worried users will get lost in the number of types and traits they might have to think about.

bal-e · 2024-11-11T11:35:02Z

src/validate.rs

+        match self {
+            Nsec3HashError::UnsupportedAlgorithm => {
+                f.write_str("Unsupported algorithm")
+            }
+            Nsec3HashError::AppendError => {
+                f.write_str("Append error: out of memory?")
+            }
+            Nsec3HashError::OwnerHashError => {
+                f.write_str("Hashing produced an invalid owner hash")
+            }
+            Nsec3HashError::CollisionDetected => {
+                f.write_str("Hash collision detected")
+            }
+        }


Factor the f.write_str() out of the match expression.

bal-e · 2025-02-03T11:48:16Z

Cargo.toml

+itertools         = "0.13.0"
+lazy_static       = { version = "1.4.0" }
+pretty_assertions = "1.4.1"
+rstest            = "0.19.0"
+rustls-pemfile    = { version = "2.1.2" }
+serde_test        = "1.0.130"
+serde_json        = "1.0.113"
+serde_yaml        = "0.9"
+socket2           = { version = "0.5.5" }
+tokio             = { version = "1.37", features = ["rt-multi-thread", "io-util", "net", "test-util"] }
+tokio-rustls      = { version = "0.26", default-features = false, features = [ "ring", "logging", "tls12" ] }
+tokio-test        = "0.4"
+tokio-tfo         = { version = "0.2.0" }
+webpki-roots      = { version = "0.26" }


Could you make this more consistent between the { version = "..." } and "..." cases?

bal-e · 2025-02-03T11:49:58Z

src/base/iana/zonemd.rs

+    /// the [ZONEMD].
+    ///
+    /// For the currently registered values see the [IANA registration]. This
+    /// type is complete as of 2024-11-29.


Reword "complete" to "up-to-date"? "Complete" makes it sound like no more additions are expected.

I think this was copied from existing domain code, i.e. aimed to be consistent.

bal-e · 2025-02-03T11:51:32Z

src/base/iana/zonemd.rs

+    /// Specifies that the SHA-384 algorithm is used.
+    (SHA384 => 1, "SHA384")
+
+    /// Specifies that the SHA-512 algorithm is used.
+    (SHA512 => 2, "SHA512")


Perhaps the doc comments should be phrased as "Use the SHA-xxx algorithm." That would be consistent with other documentation (e.g. we document what a function does, not what it is).

I think this was copied from existing domain code, i.e. aimed to be consistent.

bal-e · 2025-02-03T11:56:24Z

src/rdata/nsec3.rs

+impl<Octs> Default for Nsec3param<Octs>
+where
+    Octs: From<&'static [u8]>,
+{
+    /// Best practice default values for NSEC3 hashing.
+    ///
+    /// Per [RFC 9276] section 3.1:
+    ///
+    /// - _SHA-1, no extra iterations, empty salt._
+    ///
+    /// Per [RFC 5155] section 4.1.2:
+    ///
+    /// - _The Opt-Out flag is not used and is set to zero._
+    /// - _All other flags are reserved for future use, and must be zero._
+    ///
+    /// [RFC 5155]: https://www.rfc-editor.org/rfc/rfc5155.html
+    /// [RFC 9276]: https://www.rfc-editor.org/rfc/rfc9276.html
+    fn default() -> Self {
+        Self {
+            hash_algorithm: Nsec3HashAlg::SHA1,
+            flags: 0,
+            iterations: 0,
+            salt: Nsec3Salt::empty(),
+        }
+    }
+}


I'm not entirely comfortable with the "best-practice" value being default(). If the best practices for NSEC3 ever change, we'd have to change the default() implementation or (to avoid breaking changes) introduce a new fn new_best_practices() -> Self, and end up with a confusing interface. I'd rather have a fn best_practices() method from the beginning, and no Default impl. This also makes it more clear to end users that they're opting into a best-practices value rather than a default inherent to the type. They're also more likely to find and read the documentation, rather than just calling Default::default() and assuming that will work.

I think you make a good point.

My intent was to provide good defaults.

bal-e · 2025-02-03T13:20:55Z

src/validate.rs

+    /// The hash provider did not provide a hash for the given owner name.
+    MissingHash,


In what context could such an error occur? Can't the hash provider interface just guarantee that a hash will always be provided?

bal-e · 2025-02-03T13:22:21Z

src/validate.rs

+    /// The hashing process produced an invalid owner hash.
+    ///
+    /// See: [OwnerHashError](crate::rdata::nsec3::OwnerHashError)
+    OwnerHashError,


There isn't a good way for an end user to respond to this error. Can we find a way to guarantee that it can't happen? I'm pretty sure that any SHA-256 or SHA-384 hash will satisfy the requirements of OwnerHash.

bal-e · 2025-02-03T13:23:41Z

src/sign/config.rs

+
+    pub rrsig_validity_period_strategy: ValidityStrat,
+
+    _phantom: PhantomData<(Inner, KeyStrat, Sort)>,


Sort doesn't have to be included here, it's already used in DenialConfig.

I think I tried that and it didn't compile.

bal-e · 2025-02-03T13:26:17Z

src/sign/denial/nsec3.rs

+    pub fn fixed(ttl: Ttl) -> Self {
+        Self::Fixed(ttl)
+    }
+
+    pub fn soa() -> Self {
+        Self::Soa
+    }
+
+    pub fn soa_minimum() -> Self {
+        Self::SoaMinimum
+    }


Why do these exist? Self::Fixed, Self::Soa, and Self::SoaMinimum are exactly equivalent.

No they are not.

The RFCs don't state how the TTL of the NSEC3PARAM RR should be chosen and different tools do it at least these three different ways.

bal-e · 2025-02-03T14:05:30Z

src/sign/traits.rs

+///
+/// [`sign_zone()`]: SignableZone::sign_zone
+pub trait SignableZone<N, Octs, Sort>:
+    Deref<Target = [Record<N, ZoneRecordData<Octs, N>>]>


This bound is incredibly restrictive -- it prevents the use of this trait with any zone representation beyond those already provided (e.g. an efficient zone representation where records are stored in a B-tree). In order to be useful, these traits need to be loosened significantly, or should not be expressed as traits at all.

Ideally there would be no reference to slices or arrays at all, only iterators. This evolved out of the existing code but isn't a desirable end state. Currently it is whatever compiles and works.

ximon18 · 2025-02-03T23:06:40Z

The missing docs are just not written yet.

ximon18 · 2025-02-03T23:09:00Z

Re strategy traits, maybe. However, we use them in dnst. They exist at points where there is no one right way to do it, but a default is provided by us.

ximon18 · 2025-02-03T23:09:35Z

Re similarly named types, yes it would be good to see how to improve this.

… requirement that the input be sorted. - Return an error on collision. - Add a collision test. - More unwrap -> Err. - More comments.

ximon18 requested a review from a team October 15, 2024 14:18

ximon18 added the needs-review label Oct 15, 2024

bal-e force-pushed the dnssec-key branch from dba5a8a to 94541da Compare October 16, 2024 07:42

ximon18 changed the title ~~Initial NSEC3 generation support.~~ NSEC3 generation support. Oct 17, 2024

This was referenced Oct 17, 2024

Enhanced zone signing. #418

Merged

Add nsec3-hash command. NLnetLabs/dnst#6

Merged

bal-e force-pushed the dnssec-key branch 2 times, most recently from a79aac6 to 0f54a8d Compare October 24, 2024 14:22

bal-e reviewed Oct 30, 2024

View reviewed changes

bal-e reviewed Oct 31, 2024

View reviewed changes

ximon18 changed the base branch from dnssec-key to main November 8, 2024 12:13

tertsdiepraam force-pushed the initial-nsec3-generation branch from cf2f450 to e1c1db8 Compare November 22, 2024 14:22

ximon18 mentioned this pull request Dec 4, 2024

Add signzone command. NLnetLabs/dnst#8

Open

ximon18 added 14 commits December 5, 2024 10:52

Merge branch 'byo-signing-sorter-impl' into sortedrecords-zonemd-remo…

9607ba0

…ve-replace

Merge branch 'sortedrecords-zonemd-remove-replace' into sortedrecords…

9325956

…-zonemd-remove-replace-plus-pr444

Rename strategy fns to refer to what they are selecting more accurately.

8c2b140

Merge branch 'multiple-key-signing' into byo-signing-sorter-impl

12c252f

Merge branch 'byo-signing-sorter-impl' into sortedrecords-zonemd-remo…

e4a5bf9

…ve-replace

Merge branch 'sortedrecords-zonemd-remove-replace' into sortedrecords…

b9f9b4a

…-zonemd-remove-replace-plus-pr444

Merge branch 'main' into initial-nsec3-generation

ad795a9

Merge branch 'initial-nsec3-generation' into multiple-key-signing

33178c2

Merge branch 'multiple-key-signing' into byo-signing-sorter-impl

10193c7

Merge branch 'byo-signing-sorter-impl' into sortedrecords-zonemd-remo…

5f6f7e7

…ve-replace

Merge branch 'sortedrecords-zonemd-remove-replace' into sortedrecords…

254b52f

…-zonemd-remove-replace-plus-pr444

Make key selection more flexible. (#464)

bc68b0b

E.g. LDNS seems to consider DS and CDS and CDNSKEY resource record types as well as DNSKEY when selecting keys.

Merge branch 'multiple-key-signing' into byo-signing-sorter-impl

c7f26ac

Merge branch 'byo-signing-sorter-impl' into sortedrecords-zonemd-remo…

6b7321c

…ve-replace

ximon18 added 5 commits January 23, 2025 16:05

Normalize the generate_xxx interfaces to take config objects and retu…

56ce3b0

…rn just the record types they produce.

Fix broken doc test, restore flexible signature for Default impl for …

0680c1f

…SortedRecords.

More normalization of the generate_xxx interfaces to take config obje…

14cd78f

…cts.

Initial NSEC3 unit tests based on existing NSEC tests.

5efcccf

Fix missing feature dependency.

1660cba

Philip-NLnetLabs reviewed Jan 29, 2025

View reviewed changes

ximon18 added 2 commits January 30, 2025 15:42

Take validity time for a signature as input to signing, not from a ke…

13f8e51

…y, as the validity period required can change over the lifetime of a key and may be affected by other factors such as current signature expiration time and jitter.

Impl Display for IntendedKeyPurpose.

4910b9b

Philip-NLnetLabs reviewed Jan 31, 2025

View reviewed changes

src/sign/signatures/rrsigs.rs Outdated Show resolved Hide resolved

Philip-NLnetLabs reviewed Jan 31, 2025

View reviewed changes

src/sign/signatures/rrsigs.rs Outdated Show resolved Hide resolved

ximon18 added 3 commits January 31, 2025 17:10

Review feedback: Remove From<Dnskey> as it is not needed.

6b6588c

Remove unnecessary Send bounds.

1d950ae

Add setter for RRSIg validity period to SigningConfig.

01f542b

Philip-NLnetLabs reviewed Feb 3, 2025

View reviewed changes

src/sign/signatures/rrsigs.rs Outdated Show resolved Hide resolved

ximon18 added 3 commits February 3, 2025 14:55

Simplify NSEC unit test code.

d311371

Simplify NSEC3 unit test code and fix up the occluded test copied fro…

2d33877

…m the NSEC unit test suite with the changes required for the NSEC3 case.

bal-e reviewed Feb 3, 2025

View reviewed changes

ximon18 added 7 commits February 4, 2025 11:00

FIX: Inverted flag.

ae36053

Review feedback: Wrong comment.

1904364

FIX: Use the supplied sorter.

3c0746a

- Remove NSEC3 type bit map merging as it is not necessary due to the…

72b7785

… requirement that the input be sorted. - Return an error on collision. - Add a collision test. - More unwrap -> Err. - More comments.

Clippy.

daa6159

Remove temporary init_logging() helper fn.

f372c91

Add a test for hashing not producing the expected result.

fcc94d2

This was referenced Feb 6, 2025

NSEC3 salt strings should only be accepted if within the salt size limit. #431

Open

Implement FromStr for zonemd Scheme and Algorithm #444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NSEC3 and multiple key signing support. #416

NSEC3 and multiple key signing support. #416

ximon18 commented Oct 15, 2024 •

edited

Loading

bal-e Oct 16, 2024

bal-e Oct 16, 2024

ximon18 Oct 30, 2024

ximon18 Oct 30, 2024 •

edited

Loading

bal-e Oct 16, 2024

ximon18 Oct 30, 2024

ximon18 commented Oct 30, 2024

bal-e commented Oct 30, 2024

ximon18 commented Oct 30, 2024

bal-e Oct 31, 2024

ximon18 Oct 31, 2024

Philip-NLnetLabs Jan 29, 2025

ximon18 Jan 30, 2025

bal-e left a comment

bal-e Nov 11, 2024

bal-e Feb 3, 2025

bal-e Feb 3, 2025

ximon18 Feb 3, 2025

bal-e Feb 3, 2025

ximon18 Feb 3, 2025

bal-e Feb 3, 2025

ximon18 Feb 3, 2025

bal-e Feb 3, 2025

bal-e Feb 3, 2025

bal-e Feb 3, 2025

ximon18 Feb 3, 2025

bal-e Feb 3, 2025

ximon18 Feb 3, 2025

bal-e Feb 3, 2025

ximon18 Feb 3, 2025

ximon18 commented Feb 3, 2025

ximon18 commented Feb 3, 2025

ximon18 commented Feb 3, 2025

		let owner_hash = OwnerHash::from_octets(hash)
		.map_err(\|_\| Nsec3HashError::OwnerHashError)?;

		/// The hash provider did not provide a hash for the given owner name.
		MissingHash,


		pub rrsig_validity_period_strategy: ValidityStrat,

		_phantom: PhantomData<(Inner, KeyStrat, Sort)>,

NSEC3 and multiple key signing support. #416

Are you sure you want to change the base?

NSEC3 and multiple key signing support. #416

Conversation

ximon18 commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 commented Oct 30, 2024

bal-e commented Oct 30, 2024

ximon18 commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bal-e left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 commented Feb 3, 2025

ximon18 commented Feb 3, 2025

ximon18 commented Feb 3, 2025

ximon18 commented Oct 15, 2024 •

edited

Loading

ximon18 Oct 30, 2024 •

edited

Loading